-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bumping version to 1.5.0rc2, Add model contracts #286
Conversation
Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide. |
b9f8bff
to
28fc422
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, why tests for Galaxy are disabled? Have you investigated it?
For some tests, we're receiving this error on galaxy:
I made only these tests disabled on galaxy. I've duplicated them, decorated with |
Yes, it's known limitation. Maybe you could just adjust |
28fc422
to
dc2afc9
Compare
@hovaesco adjusted to this comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, please submit a PR to dbt docs.
Hmm wouldn't it be better to prepare PR, but submit it just after releasing dbt-trino 1.5.0 final? Maybe we shouldn't update dbt docs before releasing our stuff? To avoid situation where someone is asking about feature mentioned in docs, but not released yet |
dbt/adapters/trino/impl.py
Outdated
@@ -21,6 +22,14 @@ class TrinoAdapter(SQLAdapter): | |||
ConnectionManager = TrinoConnectionManager | |||
AdapterSpecificConfigs = TrinoConfig | |||
|
|||
CONSTRAINT_SUPPORT = { | |||
ConstraintType.check: ConstraintSupport.NOT_SUPPORTED, | |||
ConstraintType.not_null: ConstraintSupport.NOT_SUPPORTED, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this works in iceberg, it would be nice to support this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added new commit with support for not-null
constraint. All tests needed to be marked with @pytest.mark.iceberg
now, as catalog memory
does not support non-null column
create table | ||
{{ relation }} | ||
{{ get_table_columns_and_constraints() }} | ||
{{ get_assert_columns_equivalent(sql) }} | ||
{%- set sql = get_select_subquery(sql) %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please create an issue to investigate type specification in CTAS in Trino. We should validate if this is supported by the SQL spec or not because this way won't allow to hot swap the table using CREATE OR REPLACE
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I would prefer this to be implemented with DESCRIBE OUTPUT instead of splitting into a CT and INSERT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed offline, let's keep current implementation and I'll create issue which you mentioned.
This PR is about:
(High level explanation of these changes in this discussion under
Model Contracts
section)Some remarks about model contracts implementation:
It is implemented by running 2 queries:
Create table
andInsert into
(similar as in dbt-redshift - example query).It couldn't be implemeted as single CTAS query (as in dbt-snowflake - example query), as in Trino we can't
define column types in CTAS query.
dbt-core model contracts works on enforcing column names and types in table, and columns constraints:
not null
,unique
,primary key
,foreign key
,check
Trino doesn't support any of them, except that
not null
is supported in some connectors.It is a tricky one, as
not null
is supported in some connectors, but it isn't in others (memory, hive).It isn't documented in Trino docs (it isn't explicitly mentioned in connector page - hive connector docs, nor in CREATE TABLE docs)
So I'm not sure if we should support it now, as user would ask questions why it's not working with
'X' catalog, when we can't point them directly to connector documentation.
Please let me know what you think.